From e8afad4163057db38c2bccc6c36c2caac72b4f81 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Mon, 30 Nov 2015 11:29:15 -0800
Subject: [PATCH] Package/test tarballs in a temporary location

Right now a `Bomb` struct is used to attempt to ensure that broken tarballs
don't escape, but this unfortunately doesn't work for when Cargo is terminated
via other means such as ctrl-c or abnormal termination. Instead the tarball is
constructed in a temporary location and then only moved to the final location
once all checks pass.

Closes #2173
cc #2177
---
 src/cargo/ops/cargo_package.rs | 48 +++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs
index b5539c1e8..ea986ef2f 100644
--- a/src/cargo/ops/cargo_package.rs
+++ b/src/cargo/ops/cargo_package.rs
@@ -13,17 +13,6 @@ use sources::PathSource;
 use util::{self, CargoResult, human, internal, ChainError, Config};
 use ops;
 
-struct Bomb { path: Option<PathBuf> }
-
-impl Drop for Bomb {
-    fn drop(&mut self) {
-        match self.path.as_ref() {
-            Some(path) => { let _ = fs::remove_file(path); }
-            None => {}
-        }
-    }
-}
-
 pub fn package(manifest_path: &Path,
                config: &Config,
                verify: bool,
@@ -51,23 +40,32 @@ pub fn package(manifest_path: &Path,
         return Ok(None)
     }
 
-    let filename = format!("package/{}-{}.crate", pkg.name(), pkg.version());
-    let target_dir = config.target_dir(&pkg);
-    let dst = target_dir.join(&filename);
-    if fs::metadata(&dst).is_ok() { return Ok(Some(dst)) }
-
-    let mut bomb = Bomb { path: Some(dst.clone()) };
+    let filename = format!("{}-{}.crate", pkg.name(), pkg.version());
+    let dir = config.target_dir(&pkg).join("package");
+    let dst = dir.join(&filename);
+    if fs::metadata(&dst).is_ok() {
+        return Ok(Some(dst))
+    }
 
+    // Package up and test a temporary tarball and only move it to the final
+    // location if it actually passes all our tests. Any previously existing
+    // tarball can be assumed as corrupt or invalid, so we just blow it away if
+    // it exists.
     try!(config.shell().status("Packaging", pkg.package_id().to_string()));
-    try!(tar(&pkg, &src, config, &dst).chain_error(|| {
+    let tmp_dst = dir.join(format!(".{}", filename));
+    let _ = fs::remove_file(&tmp_dst);
+    try!(tar(&pkg, &src, config, &tmp_dst, &filename).chain_error(|| {
         human("failed to prepare local package for uploading")
     }));
     if verify {
-        try!(run_verify(config, &pkg, &dst).chain_error(|| {
+        try!(run_verify(config, &pkg, &tmp_dst).chain_error(|| {
             human("failed to verify package tarball")
         }))
     }
-    Ok(Some(bomb.path.take().unwrap()))
+    try!(fs::rename(&tmp_dst, &dst).chain_error(|| {
+        human("failed to move temporary tarball into final location")
+    }));
+    Ok(Some(dst))
 }
 
 // check that the package has some piece of metadata that a human can
@@ -132,9 +130,11 @@ fn check_dependencies(pkg: &Package, config: &Config) -> CargoResult<()> {
     Ok(())
 }
 
-fn tar(pkg: &Package, src: &PathSource, config: &Config,
-       dst: &Path) -> CargoResult<()> {
-
+fn tar(pkg: &Package,
+       src: &PathSource,
+       config: &Config,
+       dst: &Path,
+       filename: &str) -> CargoResult<()> {
     if fs::metadata(&dst).is_ok() {
         bail!("destination already exists: {}", dst.display())
     }
@@ -144,7 +144,7 @@ fn tar(pkg: &Package, src: &PathSource, config: &Config,
     let tmpfile = try!(File::create(dst));
 
     // Prepare the encoder and its header
-    let filename = Path::new(dst.file_name().unwrap());
+    let filename = Path::new(filename);
     let encoder = GzBuilder::new().filename(try!(util::path2bytes(filename)))
                                   .write(tmpfile, Compression::Best);
 
-- 
2.30.2